opt: fix trigger+computed column internal error #154789
Conversation
|
This change is |
|
This feels a bit hacky, but gets the job done. Probably worth thinking if there is a better way to fix this. |
There was a problem hiding this comment.
It seems that targetCols is only used to keep track of the columns directly being inserted into or being set by an update. I think the right way to fix it is actually to have addSynthesizedComputedCols skip appending to targetCols when called for a trigger. What do you think?
I think the mutationBuilder could use some refactoring. That field is really only meant to be used "locally" while building the input for an insert or an update (and is unset after that's done), but that's not really clear from the comments.
Reviewable status:
complete! 0 of 0 LGTMs obtained
I like it. I tried something similar on Friday, but I forgot why I abandoned it. Let me try again. |
This commit fixes a bug that was caused by incorrectly modifying the set and list of target columns when re-projecting computed columns after building a `BEFORE` trigger. Fixes cockroachdb#154672 Release note (bug fix): A bug has been fixed that caused internal errors for `INSERT .. ON CONFLICT .. DO UPDATE` statements when the target table had both a computed column and a `BEFORE` trigger. This bug has been present since triggers were introduced in v24.3.0.
mgartner
force-pushed
the
154672-trigger-computed-col
branch
from
8f13f74 to
f88f3b1
Compare
There was a problem hiding this comment.
@DrewKimball reviewed 7 of 7 files at r2, all commit messages.
Reviewable status:
| func (mb *mutationBuilder) addSynthesizedComputedCols(colIDs opt.OptionalColList, restrict bool) { | ||
| mb.targetColSet = mb.projectSynthesizedComputedCols(colIDs, restrict) | ||
| for col, ok := mb.targetColSet.Next(0); ok; col, ok = mb.targetColSet.Next(col + 1) { | ||
| mb.targetColList = append(mb.targetColList, col) |
There was a problem hiding this comment.
I think this assumes that table columns have ascending IDs... which is true right now, but maybe not something we want to rely on.
Here's one place where we assume that the order of columns in targetColList is the same as the order of columns in mb.outScope after projectSynthesizedComputedCols returns:
cockroach/pkg/sql/opt/optbuilder/insert.go
Lines 699 to 701 in b845bd8
The need for targetColList is dubious... Maybe we can get rid of it in the near future...
In the meantime I'm going to attempt a safer fix.
This commit fixes a bug that was caused by incorrectly modifying the set
and list of target columns when re-projecting computed columns after
building a
BEFOREtrigger.Fixes #154672
Release note (bug fix): A bug has been fixed that caused internal errors
for
INSERT .. ON CONFLICT .. DO UPDATEstatements when the targettable had both a computed column and a
BEFOREtrigger. This bug hasbeen present since triggers were introduced in v24.3.0.